Skip to content

chore(comments): Drop dead code from comments app#42174

Merged
susnux merged 1 commit intomasterfrom
chore/comments-cleanup-dead-code
Dec 13, 2023
Merged

chore(comments): Drop dead code from comments app#42174
susnux merged 1 commit intomasterfrom
chore/comments-cleanup-dead-code

Conversation

@susnux
Copy link
Contributor

@susnux susnux commented Dec 12, 2023

Summary

Drop code that is not used anymore from repository - also drop "comments" script / entry point which is unused.

Checklist

@susnux
chore(comments): Drop dead code from comments app
529dcc3 
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux added 3. to review Waiting for reviews technical debt labels Dec 12, 2023
@susnux susnux added this to the Nextcloud 29 milestone Dec 12, 2023
@susnux susnux requested review from a team, ChristophWurst, Pytal, emoral435 and skjnldsv and removed request for a team December 12, 2023 13:42
@susnux susnux requested a review from artonge December 12, 2023 16:50
artonge
artonge approved these changes Dec 12, 2023
View reviewed changes
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the use of this script?

@susnux
Copy link
Contributor Author

susnux commented Dec 13, 2023

Drone (samba) unrelated

@susnux susnux merged commit 11e27ac into master Dec 13, 2023
@susnux susnux deleted the chore/comments-cleanup-dead-code branch December 13, 2023 10:33
@susnux
Copy link
Contributor Author

susnux commented Dec 13, 2023

What was the use of this script?

Old API that was dropped long time ago it seems. The only API used by other apps is the OCA.Comments.View used by e.g. announcement center

@artonge
Copy link
Contributor

artonge commented Dec 13, 2023

I remember @nickvergessen was also afraid to lose an API for Talk

@nickvergessen
Copy link
Member

nickvergessen commented Dec 13, 2023 •
edited
Loading

Old API that was dropped long time ago it seems

It was not dropped. I prevent removal just last month....

This is public API:

server/lib/private/Comments/Manager.php

Lines 1615 to 1623 in aa5f037

/**
* Load the Comments app into the page
*
* @since 21.0.0
*/
public function load(): void {
$this->initialStateService->provideInitialState('comments', 'max-message-length', IComment::MAX_MESSAGE_LENGTH);
Util::addScript('comments', 'comments-app');
}

Can we just keep it for now?

We can deprecate and drop in 3 years if there are reasons apart from "just because"

@nickvergessen nickvergessen mentioned this pull request Dec 13, 2023
Closed
@nickvergessen
Copy link
Member

nickvergessen commented Dec 13, 2023

See #41491 (review) and further

@nickvergessen
Copy link
Member

nickvergessen commented Dec 13, 2023

Reverting in #42207 to restore the nice functionality and public API

@susnux
Copy link
Contributor Author

susnux commented Dec 13, 2023 •
edited
Loading

@nickvergessen that is a different script that is still provided. comments-app is still provided.

Meaning the public API is still there just some dead files are removed.

@blizzz blizzz mentioned this pull request Mar 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@artonge artonge artonge approved these changes

@skjnldsv skjnldsv Awaiting requested review from skjnldsv

@Pytal Pytal Awaiting requested review from Pytal

@ChristophWurst ChristophWurst Awaiting requested review from ChristophWurst

+1 more reviewer

@emoral435 emoral435 emoral435 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

3. to review Waiting for reviews technical debt

Projects

None yet

Milestone

Nextcloud 29

Development

Successfully merging this pull request may close these issues.

4 participants

@susnux @artonge @nickvergessen @emoral435